-
Notifications
You must be signed in to change notification settings - Fork 391
upcoming: [M3-9225] - Add API queries for MarketplaceV2 #13255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upcoming: [M3-9225] - Add API queries for MarketplaceV2 #13255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to this PR, but it might be worth fixing here: I noticed the factory mocks & Types don't fully match the API doc
logo_url_night_modeand its value should be updated to matchlogo_url_dark_modeproduct_countbe changed toproducts_countat L27 & L34
| import type { Filter, Params } from '@linode/api-v4'; | ||
|
|
||
| export const marketplaceQueries = createQueryKeys('marketplace', { | ||
| product: (productId: number) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now the api has changed from /marketplace/products/{id} to /marketplace/products/{id}/details
Let's verify all the fields for all the apis as per this doc: https://docs.google.com/document/d/1Iw5nUpUwtofBz5pSnr7j5jXPMBlncFVdyfzLYfnN0NI/edit?tab=t.0 |
| queryKey: [filter], | ||
| }), | ||
| paginated: (params: Params = {}, filter: Filter = {}) => ({ | ||
| queryFn: () => getMarketplaceTypes(params, filter), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be getMarketplacePartners, instead of getMarketplaceTypes
| infinite: (filter: Filter = {}) => ({ | ||
| queryFn: ({ pageParam }) => | ||
| getMarketplaceProducts( | ||
| { page: pageParam as number, page_size: 25 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should page size be 25 for products api also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimum size for a page is 25
| filter: Filter, | ||
| enabled: boolean = false, | ||
| ) => { | ||
| useQuery<ResourcePage<MarketplacePartner>, APIError[]>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to have a return statement before useQuery
| MarketplaceCategory, | ||
| MarketplacePartner, | ||
| MarketplacePartnerReferralPayload, | ||
| MarketplaceProduct, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's separate types from value imports and use import type { way instead for importing the types
| placeholderData: keepPreviousData, | ||
| }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no types defined in here like (like useMarketplaceTypesQuery, useAllMarketplaceTypesQuery, etc.). In keys we have queries defined for types api though. Hook should be added here for types.
| export const useMarketplaceProductsQuery = ( | ||
| params: Params, | ||
| filter: Filter, | ||
| enabled: boolean = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we defaulting enabled to false? it means queries won't run unless explicitly enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'd prefer to keep it this way until we have the feature flag. I'm open to changing the value if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd lean towards keeping the default as true and controlling this query by passing enabled from the consumer. That way, we only disable it when the feature flag is off, which is consistent with how we handle this in other places and is technically safer
…ub.com/harsh-akamai/manager into m3-9925-add-api-queries-to-marketplace
tvijay-akamai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harsh-akamai This looks good to me. Thanks. Approved.
| partner_id: number; | ||
| product_tags?: string[]; | ||
| short_description: string; | ||
| title_tag?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| title_tag?: string; | |
| tile_tag?: string; |
This should be tile_tag
| } | ||
|
|
||
| export interface MarketplaceCategory { | ||
| category: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| category: string; | |
| name: string; |
As per API doc, I think this should be name
| created_at: string; | ||
| created_by: string; | ||
| id: number; | ||
| logo_url_dark_mode?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| logo_url_dark_mode?: string; | |
| logo_url_dark_mode: string; |
I don't this this should be an optional field
| @@ -0,0 +1,197 @@ | |||
| import { APIError, createPartnerReferral, ResourcePage } from '@linode/api-v4'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's separate type imports from function imports
import { createPartnerReferral } from '@linode/api-v4';
import type { APIError, ResourcePage } from '@linode/api-v4';
| export const useMarketplaceProductsQuery = ( | ||
| params: Params, | ||
| filter: Filter, | ||
| enabled: boolean = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd lean towards keeping the default as true and controlling this query by passing enabled from the consumer. That way, we only disable it when the feature flag is off, which is consistent with how we handle this in other places and is technically safer
| export const marketplaceCategoryFactory = | ||
| Factory.Sync.makeFactory<MarketplaceCategory>({ | ||
| id: Factory.each((id) => id), | ||
| category: Factory.each((id) => `marketplace-category-${id}`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| category: Factory.each((id) => `marketplace-category-${id}`), | |
| name: Factory.each((id) => `marketplace-category-${id}`), |
| product_tags: ['tag1', 'tag2'], | ||
| short_description: | ||
| 'This is a short description of the marketplace product.', | ||
| title_tag: 'Marketplace Product Title Tag', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| title_tag: 'Marketplace Product Title Tag', | |
| tile_tag: 'New', |
or
| title_tag: 'Marketplace Product Title Tag', | |
| tile_tag: '60 days free trial', |
| created_at: '2024-01-01T00:00:00', | ||
| created_by: 'user1', | ||
| id: Factory.each((id) => id), | ||
| logo_url_dark_mode: 'https://www.example.com/logo-night-mode.png', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| logo_url_dark_mode: 'https://www.example.com/logo-night-mode.png', | |
| logo_url_dark_mode: 'https://www.example.com/logo-dark-mode.png', |
minor nit
| @@ -1,42 +1,59 @@ | |||
| export interface MarketplaceProductDetail { | |||
| documentation: string; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| documentation: string; | |
| documentation?: string; |
documentation appears to be an optional field as well
| category_ids: number[]; | ||
| created_at: string; | ||
| created_by: string; | ||
| details?: MarketplaceProductDetail; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but I don't see details marked as optional anywhere. Still it's worth checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Details is the part of the /products/{id}/details API only. Every other field is returned from both /products and /products/{id}/details
Cloud Manager UI test results🔺 1 failing test on test run #7 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/clone-linode.spec.ts" |
|||||||||||||||||
pmakode-akamai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @harsh-akamai for making the suggested changes - looks good to me. Could you check if we need to add a changeset for the new changes to the apiv4 package?
Changes 🔄
Added queries for the following marketplace endpoints
Scope 🚢
Upon production release, changes in this PR will be visible to:
How to test 🧪
Verification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅